[AArch64][SME] Add diagnostics for SME attributes on lambda functions #121777
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CheckFunctionDeclaration emits diagnostics if any SME attributes are used
by a function definition without the required +sme or +sme2 target features.
This patch adds similar diagnostics to CheckConstexprFunctionDefinition to
ensure this emits the same errors when attributes such as __arm_new("za")
are found without +sme/+sme2.
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-clang Author: Kerry McLaughlin (kmclaughlin-arm) ChangesCheckFunctionDeclaration emits diagnostics if any SME attributes are used Full diff: https://github.com/llvm/llvm-project/pull/121777.diff 5 Files Affected:
diff --git a/clang/include/clang/Sema/SemaARM.h b/clang/include/clang/Sema/SemaARM.h
index 8c4c56e2221301..08b03af7fd4bc6 100644
--- a/clang/include/clang/Sema/SemaARM.h
+++ b/clang/include/clang/Sema/SemaARM.h
@@ -79,6 +79,7 @@ class SemaARM : public SemaBase {
void handleNewAttr(Decl *D, const ParsedAttr &AL);
void handleCmseNSEntryAttr(Decl *D, const ParsedAttr &AL);
void handleInterruptAttr(Decl *D, const ParsedAttr &AL);
+ void CheckSMEFunctionDefAttributes(const FunctionDecl *FD);
};
SemaARM::ArmStreamingType getArmStreamingFnType(const FunctionDecl *FD);
diff --git a/clang/lib/Sema/SemaARM.cpp b/clang/lib/Sema/SemaARM.cpp
index 411baa066f7097..eafd43eb979ba0 100644
--- a/clang/lib/Sema/SemaARM.cpp
+++ b/clang/lib/Sema/SemaARM.cpp
@@ -1328,4 +1328,57 @@ void SemaARM::handleInterruptAttr(Decl *D, const ParsedAttr &AL) {
ARMInterruptAttr(getASTContext(), AL, Kind));
}
+// Check if the function definition uses any AArch64 SME features without
+// having the '+sme' feature enabled and warn user if sme locally streaming
+// function returns or uses arguments with VL-based types.
+void SemaARM::CheckSMEFunctionDefAttributes(const FunctionDecl *FD) {
+ const auto *Attr = FD->getAttr<ArmNewAttr>();
+ bool UsesSM = FD->hasAttr<ArmLocallyStreamingAttr>();
+ bool UsesZA = Attr && Attr->isNewZA();
+ bool UsesZT0 = Attr && Attr->isNewZT0();
+
+ if (FD->hasAttr<ArmLocallyStreamingAttr>()) {
+ if (FD->getReturnType()->isSizelessVectorType())
+ Diag(FD->getLocation(),
+ diag::warn_sme_locally_streaming_has_vl_args_returns)
+ << /*IsArg=*/false;
+ if (llvm::any_of(FD->parameters(), [](ParmVarDecl *P) {
+ return P->getOriginalType()->isSizelessVectorType();
+ }))
+ Diag(FD->getLocation(),
+ diag::warn_sme_locally_streaming_has_vl_args_returns)
+ << /*IsArg=*/true;
+ }
+ if (const auto *FPT = FD->getType()->getAs<FunctionProtoType>()) {
+ FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
+ UsesSM |= EPI.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask;
+ UsesZA |= FunctionType::getArmZAState(EPI.AArch64SMEAttributes) !=
+ FunctionType::ARM_None;
+ UsesZT0 |= FunctionType::getArmZT0State(EPI.AArch64SMEAttributes) !=
+ FunctionType::ARM_None;
+ }
+
+ ASTContext &Context = getASTContext();
+ if (UsesSM || UsesZA) {
+ llvm::StringMap<bool> FeatureMap;
+ Context.getFunctionFeatureMap(FeatureMap, FD);
+ if (!FeatureMap.contains("sme")) {
+ if (UsesSM)
+ Diag(FD->getLocation(),
+ diag::err_sme_definition_using_sm_in_non_sme_target);
+ else
+ Diag(FD->getLocation(),
+ diag::err_sme_definition_using_za_in_non_sme_target);
+ }
+ }
+ if (UsesZT0) {
+ llvm::StringMap<bool> FeatureMap;
+ Context.getFunctionFeatureMap(FeatureMap, FD);
+ if (!FeatureMap.contains("sme2")) {
+ Diag(FD->getLocation(),
+ diag::err_sme_definition_using_zt0_in_non_sme2_target);
+ }
+ }
+}
+
} // namespace clang
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 4001c4d263f1d2..1603b622a4faf0 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -45,6 +45,7 @@
#include "clang/Sema/ParsedTemplate.h"
#include "clang/Sema/Scope.h"
#include "clang/Sema/ScopeInfo.h"
+#include "clang/Sema/SemaARM.h"
#include "clang/Sema/SemaCUDA.h"
#include "clang/Sema/SemaHLSL.h"
#include "clang/Sema/SemaInternal.h"
@@ -12286,58 +12287,8 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
}
}
- // Check if the function definition uses any AArch64 SME features without
- // having the '+sme' feature enabled and warn user if sme locally streaming
- // function returns or uses arguments with VL-based types.
- if (DeclIsDefn) {
- const auto *Attr = NewFD->getAttr<ArmNewAttr>();
- bool UsesSM = NewFD->hasAttr<ArmLocallyStreamingAttr>();
- bool UsesZA = Attr && Attr->isNewZA();
- bool UsesZT0 = Attr && Attr->isNewZT0();
-
- if (NewFD->hasAttr<ArmLocallyStreamingAttr>()) {
- if (NewFD->getReturnType()->isSizelessVectorType())
- Diag(NewFD->getLocation(),
- diag::warn_sme_locally_streaming_has_vl_args_returns)
- << /*IsArg=*/false;
- if (llvm::any_of(NewFD->parameters(), [](ParmVarDecl *P) {
- return P->getOriginalType()->isSizelessVectorType();
- }))
- Diag(NewFD->getLocation(),
- diag::warn_sme_locally_streaming_has_vl_args_returns)
- << /*IsArg=*/true;
- }
- if (const auto *FPT = NewFD->getType()->getAs<FunctionProtoType>()) {
- FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
- UsesSM |=
- EPI.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask;
- UsesZA |= FunctionType::getArmZAState(EPI.AArch64SMEAttributes) !=
- FunctionType::ARM_None;
- UsesZT0 |= FunctionType::getArmZT0State(EPI.AArch64SMEAttributes) !=
- FunctionType::ARM_None;
- }
-
- if (UsesSM || UsesZA) {
- llvm::StringMap<bool> FeatureMap;
- Context.getFunctionFeatureMap(FeatureMap, NewFD);
- if (!FeatureMap.contains("sme")) {
- if (UsesSM)
- Diag(NewFD->getLocation(),
- diag::err_sme_definition_using_sm_in_non_sme_target);
- else
- Diag(NewFD->getLocation(),
- diag::err_sme_definition_using_za_in_non_sme_target);
- }
- }
- if (UsesZT0) {
- llvm::StringMap<bool> FeatureMap;
- Context.getFunctionFeatureMap(FeatureMap, NewFD);
- if (!FeatureMap.contains("sme2")) {
- Diag(NewFD->getLocation(),
- diag::err_sme_definition_using_zt0_in_non_sme2_target);
- }
- }
- }
+ if (DeclIsDefn && Context.getTargetInfo().getTriple().isAArch64())
+ ARM().CheckSMEFunctionDefAttributes(NewFD);
return Redeclaration;
}
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index c5a72cf812ebc9..e599ff3731a314 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -41,6 +41,7 @@
#include "clang/Sema/ParsedTemplate.h"
#include "clang/Sema/Scope.h"
#include "clang/Sema/ScopeInfo.h"
+#include "clang/Sema/SemaARM.h"
#include "clang/Sema/SemaCUDA.h"
#include "clang/Sema/SemaInternal.h"
#include "clang/Sema/SemaObjC.h"
@@ -1854,6 +1855,9 @@ bool Sema::CheckConstexprFunctionDefinition(const FunctionDecl *NewFD,
}
}
+ if (Context.getTargetInfo().getTriple().isAArch64())
+ ARM().CheckSMEFunctionDefAttributes(NewFD);
+
// - each of its parameter types shall be a literal type; (removed in C++23)
if (!getLangOpts().CPlusPlus23 &&
!CheckConstexprParameterTypes(*this, NewFD, Kind))
diff --git a/clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp b/clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp
index ec6bb6f5035784..02ebfc8060f37e 100644
--- a/clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp
+++ b/clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -std=c++23 -fsyntax-only -verify %s
// This test is testing the diagnostics that Clang emits when compiling without '+sme'.
@@ -48,3 +48,9 @@ void streaming_compatible_def2(void (*streaming_fn_ptr)(void) __arm_streaming,
// Also test when call-site is not a function.
int streaming_decl_ret_int() __arm_streaming;
int x = streaming_decl_ret_int(); // expected-error {{call to a streaming function requires 'sme'}}
+
+void sme_attrs_lambdas() {
+ [&] __arm_locally_streaming () { return; }(); // expected-error {{function executed in streaming-SVE mode requires 'sme'}}
+ [&] __arm_new("za") () { return; }(); // expected-error {{function using ZA state requires 'sme'}}
+ [&] __arm_new("zt0") () { return; }(); // expected-error {{function using ZT0 state requires 'sme2'}}
+}
|
| // Check if the function definition uses any AArch64 SME features without | ||
| // having the '+sme' feature enabled and warn user if sme locally streaming | ||
| // function returns or uses arguments with VL-based types. | ||
| void SemaARM::CheckSMEFunctionDefAttributes(const FunctionDecl *FD) { |
Collaborator
There was a problem hiding this comment.
Should this be a static member function instead? (would have to take the ASTContext as parameter)
Contributor
Author
There was a problem hiding this comment.
I've changed this to static and added Sema as a parameter, so that the function can still use Diag() for emitting the diagnostics.
Contributor
There was a problem hiding this comment.
It's a bit inconsistent with everything else, the intent is to use ARM().CheckSMEFunctionDefAttributes(FD)
(The reason we did that was to split Sema into multiple classes/files)
Contributor
There was a problem hiding this comment.
It seems like an ARM-specific function, so you correctly placed it in SemaARM, and it's referenced in SemaDecl and SemaDeclCXX, so it can't be static. Corentin is correct that ARM().CheckSMEFunctionDefAttributes(FD) is the way to use it from outside of SemaARM.
The current state of the changes seems correct to me.
Collaborator
There was a problem hiding this comment.
Thanks for your feedback, I didn't realise that there was already precedent for using ARM().Check...
| @@ -1,4 +1,4 @@ | |||
| // RUN: %clang_cc1 -triple aarch64-none-linux-gnu -fsyntax-only -verify %s | |||
| // RUN: %clang_cc1 -triple aarch64-none-linux-gnu -std=c++23 -fsyntax-only -verify %s | |||
Collaborator
There was a problem hiding this comment.
What about this change requires c++23?
Contributor
Author
There was a problem hiding this comment.
I think adding attributes to lambdas before the parameter list was only allowed in C++23. If I don't pass this flag, the error emitted is instead "'attribute' in this position is a C++23 extension".
| int x = streaming_decl_ret_int(); // expected-error {{call to a streaming function requires 'sme'}} | ||
|
|
||
| void sme_attrs_lambdas() { | ||
| [&] __arm_locally_streaming () { return; }(); // expected-error {{function executed in streaming-SVE mode requires 'sme'}} |
Collaborator
There was a problem hiding this comment.
nit: the function doesn't use anything from the parent's scope.
Suggested change
| [&] __arm_locally_streaming () { return; }(); // expected-error {{function executed in streaming-SVE mode requires 'sme'}} | |
| [] __arm_locally_streaming () { return; }(); // expected-error {{function executed in streaming-SVE mode requires 'sme'}} |
MacDue
reviewed
Jan 6, 2025
cor3ntin
reviewed
Jan 6, 2025
clang/lib/Sema/SemaDeclCXX.cpp
Outdated
Comment on lines
1858
to
1860
| if (Context.getTargetInfo().getTriple().isAArch64()) | ||
| ARM().CheckSMEFunctionDefAttributes(NewFD); | ||
|
|
Contributor
There was a problem hiding this comment.
This seems like a weird place to do that (afaict this has nothing to do with constexpr). I suspect ActOnStartOfLambdaDefinition - after the ProcessDeclAttributes call might be a better place
Contributor
Author
There was a problem hiding this comment.
Thanks, ActOnStartOfLambdaDefinition does seem like a better place to check the attributes.
- Move CheckSMEFunctionDefAttributes call to ActOnStartOfLambdaDefinition
kmclaughlin-arm
changed the title
sdesmalen-arm
approved these changes
Jan 9, 2025
| // Check if the function definition uses any AArch64 SME features without | ||
| // having the '+sme' feature enabled and warn user if sme locally streaming | ||
| // function returns or uses arguments with VL-based types. | ||
| void SemaARM::CheckSMEFunctionDefAttributes(const FunctionDecl *FD) { |
Collaborator
There was a problem hiding this comment.
Thanks for your feedback, I didn't realise that there was already precedent for using ARM().Check...
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/9473 Here is the relevant piece of the build log for the reference |
BaiXilin
pushed a commit
to BaiXilin/llvm-project
that referenced
this pull request
Jan 12, 2025
…llvm#121777) CheckFunctionDeclaration emits diagnostics if any SME attributes are used by a function definition without the required +sme or +sme2 target features. This patch moves these diagnostics to a new function in SemaARM and also adds a call to this from ActOnStartOfLambdaDefinition.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CheckFunctionDeclaration emits diagnostics if any SME attributes are used
by a function definition without the required +sme or +sme2 target features.
This patch adds moves these diagnostics to a new function in SemaARM and
also adds a call to this from ActOnStartOfLambdaDefinition.